fix(#6420): make ImportProfile concurrency-safe with hash-based caching - #6421
Conversation
|
🤖 Finished Review · ✅ Success · Started 5:04 PM UTC · Completed 5:15 PM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Previous runLooks good to me |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
ImportProfile unconditionally deleted a provider profile by ID before re-importing, causing race conditions when multiple fullsend run invocations shared the same gateway. One process could delete a profile that another had just imported, causing EnsureProvider to fail with "unsupported provider type or profile". Mirror the hash-based idempotency pattern from ImportProfiles: compute a SHA-256 digest of the profile file, compare against a temp-file cache keyed by profile ID, and skip delete+reimport when the hash matches. On cache miss or content change, proceed with delete+reimport as before, writing the cache on success (including when a parallel process already imported the profile). Add hashProfileFile and profileFileCachePath helpers as single-file analogs of hashProfileDir and profileCachePath. Update existing tests to use real temp files (required now that ImportProfile reads the file for hashing). Add tests for cache hit/miss, cache write on success and already-exists, concurrent access, and the new helper functions. Closes #6420
98fed1e to
1b57dd4
Compare
|
🤖 Finished Review · ✅ Success · Started 2:52 AM UTC · Completed 3:06 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 3:19 AM UTC · Completed 3:36 AM UTC Commit: |
Retro: PR #6421 — make ImportProfile concurrency-safe with hash-based cachingWorkflow quality: StrongThis was an efficient, single-iteration workflow. Issue #6420 was filed at 16:44 UTC on Aug 20, triaged by 16:49, and the code agent opened PR #6421 by 17:00 — 16 minutes from issue to PR. The review agent approved with one correctly-calibrated low-severity TOCTOU finding at 17:15. No fix agent was needed. A human reviewer ( Highlights:
Evidence for existing open issues#5388 — empty expected app client_id in pre-fetch-prior-review: The rebase at 02:50 UTC triggered a second review run (32441244813). The #4401 / #4960 — rebase-only force-push re-review waste: The force push was a rebase with no code changes (same author date, same commit message, same diff). The second review spent 14.5 minutes re-analyzing identical code. Even with the provenance fix (#5388), detecting rebase-only pushes would eliminate this class of waste entirely. #2810 (closed Aug 13) — code agent coverage self-check: The code agent used No new proposalsAll improvement opportunities identified map to existing open or recently closed issues. The workflow executed well — fast triage-to-code handoff, correct first-attempt fix, appropriate review calibration, zero rework iterations. |
The #6421 hash-based cache reduced the ImportProfile race from 4/4 to 1/4 failures but did not eliminate it: when all parallel fullsend run processes start simultaneously (no cache file yet), multiple processes enter the non-atomic delete+reimport path. A concurrent EnsureProvider call that lands between delete and reimport finds no profile and fails with "unsupported provider type or profile". Two layered fixes: 1. flock in ImportProfile: the delete+reimport critical section is now protected by a cross-process file lock keyed by profile id. Only one process mutates the profile at a time. Processes that block on the lock re-check the cache after acquiring it (double-check pattern) and skip the import entirely if the winner already wrote the cache. 2. Retry in EnsureProvider: the specific "unsupported provider type or profile" error is treated as transient and retried up to 3 times with 500ms backoff. This provides defense-in-depth for any remaining timing edge cases. Non-transient errors are returned immediately without retry. Note: golangci-lint could not run in sandbox (not installed). go vet passed. Pre-commit could not run (network access blocked in sandbox); the post-script runs it authoritatively. Closes #6435
The #6421 hash-based cache reduced the ImportProfile race from 4/4 to 1/4 failures but did not eliminate it: when all parallel fullsend run processes start simultaneously (no cache file yet), multiple processes enter the non-atomic delete+reimport path. A concurrent EnsureProvider call that lands between delete and reimport finds no profile and fails with "unsupported provider type or profile". Two layered fixes: 1. flock in ImportProfile: the delete+reimport critical section is now protected by a cross-process file lock keyed by profile id. Only one process mutates the profile at a time. Processes that block on the lock re-check the cache after acquiring it (double-check pattern) and skip the import entirely if the winner already wrote the cache. 2. Retry in EnsureProvider: the specific "unsupported provider type or profile" error is treated as transient and retried up to 3 times with 500ms backoff. This provides defense-in-depth for any remaining timing edge cases. Non-transient errors are returned immediately without retry. Note: golangci-lint could not run in sandbox (not installed). go vet passed. Pre-commit could not run (network access blocked in sandbox); the post-script runs it authoritatively. Closes #6435
ImportProfiles (batch) performed delete+reimport without flock protection, causing races under parallel execution. When multiple processes saw a hash cache miss simultaneously, each deleted and reimported the same profiles, and concurrent EnsureProvider calls hit "unsupported provider type or profile" during the delete window. Add the same flock serialization pattern that ImportProfile (singular) already uses: acquire an exclusive file lock keyed by directory path, double-check the hash cache after acquisition, then perform delete+reimport inside the critical section. This is the third instance of this race class, after #6421 and #6437. Add profileDirLockPath helper for directory-keyed lock paths and a concurrent-safety test using the same marker-file technique as TestImportProfile_FlockSerializesConcurrent. Note: pre-commit could not run (sandbox network policy blocked git fetch). go vet passed. golangci-lint was not available in the sandbox. Closes #6448
2 participants
Summary
Make
ImportProfileconcurrency-safe by adding hash-based caching, mirroring the existing pattern inImportProfiles. When multiplefullsend runinvocations share the same gateway,ImportProfilepreviously did an unconditional delete-then-reimport that caused race conditions — one process could delete a profile another had just imported, causing downstreamEnsureProviderfailures.Related Issue
Changes
ImportProfilekeyed by profile ID in a temp filehashProfileFileandprofileFileCachePathhelper functionsTesting
TestImportProfile_*tests pass with updated profile file pathsTestImportProfile_SkipsWhenCacheMatchesverifies cache hit pathTestImportProfile_ReimportsWhenCacheDiffersverifies cache miss pathTestImportProfile_WritesCacheOnSuccessverifies cache write after importTestImportProfile_WritesCacheOnAlreadyExistsverifies cache write on parallel importTestImportProfile_ConcurrentAccesswith 12 goroutines verifies safety under concurrent accessgo test -race ./internal/sandbox/...passesgo vet ./internal/sandbox/...passesCloses #6420
Post-script verification
agent/6420-import-profile-concurrency)2c7812ac854a60d19d4be0c1eeed49c69bb15976..HEAD)